Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KDMqtt #45

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

KDMqtt #45

wants to merge 20 commits into from

Conversation

marcothaller
Copy link
Contributor

Draft PR to gather feedback / review of KDMqtt.

@MiKom
Copy link
Member

MiKom commented Nov 26, 2024

I will add more comments later, but first quick one is about logging.
I think it's better to use SPDLOG_LOGGER_* macros for logging because they already support printing the source location. I'd also create a dedicated logger for the mqtt system (see core_application.cpp)

So, you then can replace this:

spdlog::error("MqttClient::EventLoopHook::engage() - EventLoopHook is not initialized. Call MqttClient::EventLoopHook::init() first.");

with this:

SPDLOG_LOGGER_ERROR(m_logger, "EventLoopHook is not initialized. Call MqttClient::EventLoopHook::init() first.");

@MiKom
Copy link
Member

MiKom commented Nov 26, 2024

It seems that you also need to add the dependency installation in the .github/workflows/linters.yml file


if(WIN32)
# Deployment: On Windows, copy all DLLs from the mosquitto install directory next to the application binary so that they're found.
if(BUILD_INTEGRATION_MQTT AND MOSQUITTO_RUNTIME_DLLS)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BUILD_INTEGRATION_MQTT doesn't seem to exist anymore so this condition is always false on Windows.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be Mosquitto_FOUND, I'll fix this.

CMakeLists.txt Outdated
@@ -60,6 +60,7 @@ endif()
add_subdirectory(src/KDUtils)
add_subdirectory(src/KDFoundation)
add_subdirectory(src/KDGui)
add_subdirectory(src/KDMqtt)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd make building KDMqtt optional behind an option. And this option should be by default OFF on Windows as currently, the example doesn't seem to work. Probably something's wrong with the fd notifiers? Dunno. We can create an issue to fix it.

Our current target is Linux anyway so for now it should be fine to have KDMqtt working on Linux only.

Some KDUTILS_BUILD_MQTT_SUPPORT

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I forgot about the fd notifier issue on Windows.

@@ -43,7 +46,20 @@ jobs:
sudo apt update -qq
sudo apt install -y libxkbcommon-dev libxcb-xkb-dev \
libxkbcommon-x11-dev wayland-scanner++ wayland-protocols \
libwayland-dev xvfb ninja-build
libwayland-dev xvfb ninja-build \
libmosquittopp-dev
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC we're not using the C++ mosquitto wrapper but rather the C API directly. In this case, this should be libmosquitto-dev

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, we are using the C API, therefore libmosquitto-dev is correct.

@@ -15,7 +15,7 @@ jobs:
os:
- ubuntu-24.04
- ubuntu-22.04
- ubuntu-20.04
#- ubuntu-20.04 -> disabled during introduction of KDMqtt
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still the case, that it doesn't work on 20.04?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I see it, mosquitto v1.6.9 is the latest version available in Ubuntu 20.04:
https://launchpad.net/ubuntu/focal/+package/libmosquitto-dev
Without additional changes to KDMqtt the project won't compile on 20.04

I'd suggest to disable KDMqtt build via KDUTILS_BUILD_MQTT_SUPPORT for os: ubuntu 20.04, document this in the README and re-enable the os in build.yml.

What do you think?

You can find setup instructions and download links at

<https://mosquitto.org/download/>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also encourage the user to look at .github/workflows/build.yml

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea!


target_include_directories(
KDMqtt
PUBLIC $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/../..>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this makes mosquitto_wrapper.h a public header. Shouldn't we rather make it an implementation detail and private?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, keeping the header private makes perfect sense.

friend class MqttUnitTestHarness;

public:
MqttClient(const std::string &clientId, bool cleanSession = true, bool verbose = false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather turn the two bool parameters into a single Flags parameter for better readability on call site.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like your suggestion and with change the parameter accordingly.

: m_verbose{ verbose }
{
if (!MqttLib::instance().isInitialized()) {
spdlog::warn("MqttClient::MqttClient() - CTOR called before MqttLib::init(). Initialize lib before instantiating MqttClient object!");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an error IMO. The class can't really function properly without MqttLib being initialized.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, we should use the error category here.


private:
MqttLib();
~MqttLib() = default;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this class should call cleanup() on destruction. So that the user doesn't have to think about it (case in point: cleanup is not done in the example :) ).

According to mosquitto docs, mosquitto_lib_cleanup cannot fail so we can disregard its return value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, we should call cleanup() here. :)


KDBindings::Signal<> error;

virtual int setTls(const File &cafile) = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also add the function that lets the user use the system certificate store for TLS.

I think that by default, we should use the system store by calling mosquitto_int_option with the MOSQ_OPT_TLS_USE_OS_CERTS option and let the user opt out of it somehow.

But also, remember to document that it only works on Linux because on Windows, OpenSSL used by mosquitto doesn't use the system store by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I'll add that functionality.


if (!m_isInitialized) {
result = m_mosquittoLib->init();
const auto hasError = checkMosquittoResultAndDoDebugPrints(result, "MqttLib::init()");
Copy link
Member

@MiKom MiKom Nov 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is quite verbose and requires us to re-state the function name. I think we could replace it with a macro that would automatically handle source location and honour SPDLOG compile-time log enabling/disabling:

#if SPDLOG_ACTIVE_LEVEL <= SPDLOG_LEVEL_ERROR
#define LOG_MOSQUITTO_ERROR(logger, file, line, func, error_string) \
    (logger)->log(spdlog::source_loc{ file, line, func }, spdlog::level::err, error_string)
#else
#define LOG_MOSQUITTO_ERROR (void)0
#endif

static bool checkAndLogMosquittoResult(std::shared_ptr<spdlog::logger> logger, int ec, const char *file, int line, const char *func)
{
    const bool isError = (ec != MOSQ_ERR_SUCCESS);
    if (isError) {
        LOG_MOSQUITTO_ERROR(logger, file, line, func, mosquitto_strerror(ec));
    }
    return isError;
}
#define CHECK_AND_LOG_MOSQUITTO_RESULT(ec) checkAndLogMosquittoResult(m_logger, ec, __FILE__, __LINE__, __FUNCTION__)

Then this callsite becomes:

const auto hasError = CHECK_AND_LOG_MOSQUITTO_RESULT(result);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once we go to c++20 the macros of course will be gone as we'll be able to replace this with std::source_location

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed this in a new commit but didn't quite stick to your suggestion.

@marcothaller marcothaller force-pushed the kdmqtt branch 4 times, most recently from 1eeaff7 to b5ecf69 Compare February 3, 2025 07:24
KDMqtt provides a cross-platform MQTT client solution
based on mosquitto.
KDMqtt seamlessly integrates into KDFoundation's
`CoreApplication` and comes with an API featuring
properties and signals using KDBindings.
Adds a very basic MQTT client example.
Examlple shows how to use and integrate KDMqtt
in you KDFoundation CoreApplication.

Example connects to broker at test.mosquitto.org,
subscribes to mytopic as soon as connected to the
broker and publishes a message for mytopic as soon
as subscribing to the topic is done. The received
message is then printed using spdlog::info.

This example also outlines the steps neccessary
when deploying an application using KDMqtt on
windos (copying DLLs next to the binary).
In KDMqtt we use `mosquitto_ssl_get()`
which was added to mosquitto 2020-09-10
and is first available in v2.0.20

Ubuntu 20.04 ships with mosquitto v1.6.9

Therefore, without changes to KDMqtt,
KDMqtt cannot be built on Ubuntu 20.04 atm.
Disable CI build (for now).
* add missing header include
* add macOS specific paths to FindMosquitto.cmake
* (re)enable CI run on macOS
Adds task to artificially invoke event handlers while
the connection is being established after calling
mosquitto_connect_async.
Using mosquitto_connect_async apparently does not work
in case one monitors the client socket themself and the
connection uses TLS.
Reading the mosquitto API docs, I am not sure if this
is a bug or not. Though my hunch would be, that this is
a bug, since using mosquitto_connect_async works in case
the client socket is monitored and the connection
DOES NOT use TLS.
Other people have encountered similar behaviour and have
mentioned it on GitHub:
e.g. eclipse-mosquitto/mosquitto#990
To avoid the use of the blocking mosquitto_connect
I decided to use this workaround / hack to be able to
use the non-blocking mosquitto_connect_async.
Thus far I have not encountered any downsides using the
workaround.
Reflect ownership by using a unique pointer.
Fixes potential leak of client object.
- adds option to switch between encrypted and unencrypted
  connections to the broker in the example
- default option is set to: encrypted connection
- adds download and deployment of mosquitto.org.crt to cmake
  for the example
Handle logging verbosity via spdlog categories only.
- adds factory method to MqttLib
- makes MqttClient CTOR protected
- allows MqttLib to access protected methods
MqttLib class is responsible for orchestrating
different aspects of MQTT:
- creating MqttClient instances
- init/deinit of mosquitto library
It will soon also own a logger instance for
the MQTT system and provide that to the
individual client instances.

Therefore we think MqttManager is a more
descriptive class name.
The fact that some of the functionality is
based on library functions is rather an
implementation detail worth hiding, than a
reason to keep the `Lib` suffix.
@marcothaller marcothaller marked this pull request as ready for review February 3, 2025 13:41
Comment on lines 162 to 165
virtual int publish(int *msgId, const char *topic, const ByteArray *payload = nullptr, QOS qos = QOS::AT_MOST_ONCE, bool retain = false) = 0;

virtual int subscribe(const char *pattern, QOS qos = QOS::AT_MOST_ONCE) = 0;
virtual int unsubscribe(const char *pattern) = 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace const char* with const std::string&

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

- introduce enum to represent quality-of-service values
- make port values of type uint16_t
- consistently handle MQTT payloads as KDUtils::ByteArray
- consistently pass strings as `const std:string&`
@marcothaller marcothaller force-pushed the kdmqtt branch 4 times, most recently from 47ad12e to 66aa0a0 Compare February 4, 2025 22:01
Uses FakeIt single header mocking framework with doctest.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants